-
-
Notifications
You must be signed in to change notification settings - Fork 465
feat: merge tombstone and native sdk events #5037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Build / dependencies / internal 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| d217708 | 409.83 ms | 474.72 ms | 64.89 ms |
| abf451a | 332.82 ms | 403.67 ms | 70.85 ms |
| d15471f | 286.65 ms | 314.68 ms | 28.03 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| fc5ccaf | 279.11 ms | 353.34 ms | 74.23 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
| 1edbdfa | 364.77 ms | 450.29 ms | 85.52 ms |
| d15471f | 294.13 ms | 399.49 ms | 105.36 ms |
| 27d7cf8 | 306.76 ms | 366.66 ms | 59.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| abf451a | 1.58 MiB | 2.20 MiB | 635.29 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
| 1edbdfa | 1.58 MiB | 2.20 MiB | 635.34 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
markushi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking quite promising already, I think this is the right track. Left a few comments for further discussion.
| */ | ||
| private static void setNativeCrashCorrelationId( | ||
| final @NotNull Context context, final @NotNull SentryAndroidOptions options) { | ||
| final String correlationId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As anyone could call this API to store information, it might be a good idea to ensure it's our data we're reading back later. I guess a simple prefix could work e.g. sentry-app-run-<uuid>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, and this is only meant as a proposal that survived from 2022:
processStateSummaryis global and generally owned by the app- If we write into this, we should first check if there is already something in there
- Even if nothing was in there, any later code can overwrite it without us knowing anything about it
- The maximum length is 128 bytes, and writing into it is free-form, so there is no way for binary stability except if we communicate that Sentry owns this if you use the SDK (not sure if that is a good idea).
- If we prefix it with something generic like
sentry-app-run-, it shows that this might be used beyond native crash correlation (it will be accessible from anyApplicationExitInfo).
I implemented this because we considered it a stable correlation mechanism back then, but I think timestamp correlation is stable enough and doesn't depend on app-owned storage.
It's likely a good idea to drop this altogether, and if we introduce it, maybe together with other ApplicationExitInfo handlers?
| return new ApplicationExitInfoHistoryDispatcher.Report(event, hint, tombstoneHint); | ||
| } | ||
|
|
||
| private SentryEvent mergeNaiveCrashes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private SentryEvent mergeNaiveCrashes( | |
| private SentryEvent mergeNativeCrashes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
| .getLogger() | ||
| .log(SentryLevel.DEBUG, "Scanning %d files in outbox for native events.", files.length); | ||
|
|
||
| for (final File file : files) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried of ending up with too many envelopes in memory here and potentially running into OOMs.
| boolean deletionSuccess = nativeEventCollector.deleteNativeEventFile(matchingNativeEvent); | ||
|
|
||
| if (deletionSuccess) { | ||
| event = mergeNaiveCrashes(matchingNativeEvent.getEvent(), event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any specific information a native crash event could hold which is not available through the tombstone? I guess this is mainly about user-added context like e.g. breadcrumbs, right?
📜 Description
This implements the second aspect of the tombstone integration: merging tombstone events with crash events coming from sentry-native. It does this in the following way:
OutboxSender, but instead letTombstoneIntegrationown the outbox folder before theOutboxSenderoperates on it (i.e., we rely on integration instantiation order and the fact that all async integration work is done on a single-threaded executor service).NativeEventCollectorimplements the duplication wrtOutboxSender, but specifically for handling envelopes that contain native crashesOutboxSenderlater, that change wouldn't be a considerable effort.sentry-native, so for now it's only exposed but not used.TombstonePolicysearches, matches, and merges eventsdebugMeta,exception, andthreadsThis is primarily an end-to-end implementation to discuss the trade-offs listed.
💡 Motivation and Context
This is the last essential step before releasing the tombstone integration to a wider audience. It maps directly to
Implement merge mechanism with the Native SDK envelope
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
OutboxSenderand surrounding infra more generically (we could introduce a callback for the core of theOutboxSenderdecision and set up integrations that use it accordingly)EventProcessorfor merge events?)